Move notification handler registrations to capabilities #207
Conversation
There was a problem hiding this comment.
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/ModelContextProtocol.Tests/SseIntegrationTests.cs:233
- The square bracket initializer syntax used for NotificationHandlers may not be standard C#. Consider replacing it with a conventional collection initializer (e.g., 'new [] { ... }' or 'new List<KeyValuePair<string, Func<JsonRpcNotification, Task>>>() { ... }') to ensure compatibility.
NotificationHandlers = [new("test/notification", (args) =>
tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs
Show resolved
Hide resolved
Someone can also do it on their own, just by adding a handler that reads from any collection they want to subsequently mutate. That single handler will be invoked and can effectively proxy the call to any number of handlers elsewhere. |
Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.
stephentoub
force-pushed
the
movenotificationregistration
branch
from
c57d65d to
824d01d
Compare
|
I've been thinking more about this. This is a good direction over what was there before, which prohibited removal of added handlers, limiting their use. But if we instead allow removal, it becomes much more useful. Like with CancellationToken.Register, it the enables transiently listening for certain notifications, so for example you could create a progress token, hook up a notification handler that pays attention to only that token, then start the operation using that token, and then after the operation completes, remove that handler. It's hard to imagine how this could be achieved in a general fashion against an arbitrary IMcpClient/Server in other ways. Given that, I'm going to move back in that direction, putting it back onto the interface, but with a shape like CT.Register that enables scoped removal. cc: @halter73, @eiriktsarpalis, @PederHP |
Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.